added virtualenv_name config #373
Conversation
There was a problem hiding this comment.
Thanks for the PR! This just needs a few minor details and it'll be good to go.
One thing that this could use that isn't present is docs. Please add a little blurb about this setting to our quickstart doc section on customizing virtualenv creation
streamparse/cli/common.py
Outdated
| @@ -106,6 +106,14 @@ def add_override_name(parser): | |||
| 'duplicate the topology file.') | |||
|
|
|||
|
|
|||
| def add_virtualenv_name(parser): | |||
There was a problem hiding this comment.
It does not appear that you ever actually use this. I also think this probably makes more sense a config.json setting, like you're using it later anyway, since this exists for the use case where you have a single virtualenv you want shared by all topologies.
We might also want a warning for people doing this, since if you're sharing a virtualenv between topologies and you update the virtualenv without stopping all of those topologies, you could end up with weird errors from the code changing right underneath the other topology.
streamparse/cli/submit.py
Outdated
| @@ -171,11 +171,12 @@ def submit_topology(name=None, env_name=None, options=None, force=False, | |||
|
|
|||
| # If using virtualenv, set it up, and make sure paths are correct in specs | |||
| if use_venv: | |||
| virtualenv_override_name = env_config.get('virtualenv_name', override_name) | |||
There was a problem hiding this comment.
Nitpick: I'd rather we just stick with virtualenv_name like the CLI option is called.
streamparse/cli/update_virtualenv.py
Outdated
| @@ -51,24 +51,24 @@ def _create_or_update_virtualenv(virtualenv_root, | |||
| run("rm {}".format(temp_req)) | |||
|
|
|||
|
|
|||
| def create_or_update_virtualenvs(env_name, topology_name, override_name=None, | |||
| def create_or_update_virtualenvs(env_name, topology_name, virtualenv_override_name=None, | |||
There was a problem hiding this comment.
Nitpick: Same renaming should happen here.
|
Thank you @dan-blanchard for reviewing. I've made the changes, please let me know if there's anything that I might have missed. |
Introduced
virtualenv_namein the config.json which overrides the default virtualenv(topology name) that streamparse uses when topologies are deployed.This would take precedence over topology
override_nameand will only be used whenuse_virtualenvis True.Issue 371